You can extract repeated logic into a function.

1. This pattern, I see in Mint and Burn.

liquidityAmount := u256.MustFromDecimal(_liquidityAmount)
if liquidityAmount.IsZero() {
    panic(addDetailToError(
        errZeroLiquidity,
        ufmt.Sprintf("pool.gno__Mint() || liquidityAmount == 0"),
    ))
}

2. This is repeated in Mint, Burn and Swap.

if common.GetLimitCaller() {
    caller := std.PrevRealm().Addr()
    if err := common.PositionOnly(caller); err != nil {
        panic(addDetailToError(
            errNoPermission,
            ufmt.Sprintf("pool.gno__Mint() || only position(%s) can call pool mint(), called from %s", consts.POSITION_ADDR, caller.String()),
        ))
    }
}

3. Across all functions

pool := GetPool(token0Path, token1Path, fee)

4. This was used Swap.

if _amountSpecified == "0" {
    panic(addDetailToError(
        errInvalidSwapAmount,
        ufmt.Sprintf("pool.gno__Swap() || amountSpecified == 0"),
    ))
}

Can you also try to break down the Mint, Burn, Collect and Swap functions as small as possible?
I know it's a WIP, but it needs more unit tests. Add them.

@dongwon8247

You can extract repeated logic into a function.

1. This pattern, I see in `Mint` and `Burn`.

2. This is repeated in `Mint`, `Burn` and `Swap`.

I agree with integrating panic condition checks like 1 and 2 into a single function. However, it seems difficult to integrate them immediately since each function currently outputs different error messages. We plan to improve how we manage error messages and error codes, and I think it would be better to handle this integration when the error messages are reorganized at that time.

Proposed Idea: https://www.notion.so/Contract-Refactoring-14317eb23ada80c4bdd6c1ca7b2349bd?pvs=4#15017eb23ada802abf80d47c9e90eea7

3. Across all functions

pool := GetPool(token0Path, token1Path, fee)

...
Can you also try to break down the Mint, Burn, Collect and Swap functions as small as possible? I know it's a WIP, but it needs more unit tests. Add them.

While the line calling the GetPool function is clearly repeated, extracting it into a separate function seems unnecessary. I believe this could actually reduce readability by making it harder to locate where GetPool is being called for each function.

Regarding this, I haven't made many modifications to the entry functions except for the Swap function. This is because the structure and implementation of these functions has appropriate readability ¹, and there's no reusable logic, so I determined it unnecessary as increasing the number of functions could potentially lead to cognitive overhead.

However, as you mentioned, the Swap function had state changes and calculations mixed together, so I applied refactoring to separate them as much as possible for testing purposes. please check this commit: 0b6d9d4

And for the test, don't worry

if _amountSpecified == "0" {
    panic(addDetailToError(
        errInvalidSwapAmount,
        ufmt.Sprintf("pool.gno__Swap() || amountSpecified == 0"),
    ))
}

The logic for checking if amountSpecified is "0" appears to be a pattern found in other contracts as well, so it must be extract it into a common package.

There is no test code for position_update.gno.

@onlyhyde
added 19f1a8e

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud